Expand documentation for the use and reproducibility of side effects functions in qenv #280
Conversation
|
I have read the CLA Document and I hereby sign the CLA 2 out of 3 committers have signed the CLA. |
|
Code Coverage SummaryDiff against mainResults for commit: c16c576 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 14 suites 6s ⏱️ Results for commit 23dc660. |
Unit Tests Summary 1 files 14 suites 6s ⏱️ Results for commit c16c576. ♻️ This comment has been updated with latest results. |
Unit Test Performance DifferenceAdditional test case details
Results for commit e238907 ♻️ This comment has been updated with latest results. |
llrs-roche
left a comment
llrs-roche
left a comment
There was a problem hiding this comment.
I've left a couple of comments.
While thinking about this PR I thought on a different approach to solve the issue.
What if we link any line of code on qenv not linked to any other variable (or those on side_effect_functions) are linked to all other variables?
This would require extract the code of all variables and compare it with the full code. Any expression missed for one variable is a side-effect call that could be dealt with.
library("teal.code")
q <- within(qenv(), {
set.seed(45)
a <- runif(1)
b <- runif(2)
c <- a+b
})
cat(get_code(q, names = "c"))
#> a <- runif(1)
#> b <- runif(2)
#> c <- a + b
cat(get_code(q, names = "b"))
#> b <- runif(2)
cat(get_code(q, names = "a"))
#> a <- runif(1)Created on 2025-12-05 with reprex v2.1.1
So that the order of the detected expression is kept:
cat(get_code(q, names = "c"))
#> set.seed(45)
#> a <- runif(1)
#> b <- runif(2)
#> c <- a + b
cat(get_code(q, names = "b"))
#> set.seed(45)
#> b <- runif(2)
cat(get_code(q, names = "a"))
#> set.seed(45)
#> a <- runif(1)
|
Related to the proposal above and the opposite of the issue, some code even if not related to the variable is added when requesting code only for the variable (might be worth to open a different issue): library("teal.code")
packageVersion("teal.code")
#> [1] '0.7.0'
q <- within(qenv(), {
set.seed(45)
library("lintr")
library("lintr")
a <- runif(1)
b <- runif(2)
c <- a+b
d <- 5
})
cat(get_code(q, names = "d"))
#> library("lintr")
#> library("lintr")
#> d <- 5
cat(get_code(q, names = "c"))
#> library("lintr")
#> library("lintr")
#> a <- runif(1)
#> b <- runif(2)
#> c <- a + b
cat(get_code(q, names = "b"))
#> library("lintr")
#> library("lintr")
#> b <- runif(2)
cat(get_code(q, names = "a"))
#> library("lintr")
#> library("lintr")
#> a <- runif(1)Created on 2025-12-05 with reprex v2.1.1 |
|
Many thanks for the comments. I have addressed two of them. The teal.gallery, I could check if some qenv has side effects functions. If it has, we could refactor the qenv to avoid warnings if they appear. The posiblity to add more code than necessary exists, but I only observe good code reporting in your last example (maybe I am missing something). About the design, is it really necessary on a qenv to change the control of a random number generator more than once? Is not that we should ban it. Thus, we could create good practices in qenv that could be only to use the same side effect function once unless is very necessary. And if we define a good qenv style, we can apply specific lintr. For the within, my pick is that either we do not allow comments and linking for the moment or we could set the side effect functions as a method parameter and they would be called with all variables as the library calls. |
|
My personal view is that none of the side effects should be detected automatically as this might not be intended and may lead to a lot of confusion. We have |
osenan
force-pushed
the
improve-doc-within@main
branch
from
2b7233e to
eb903f8
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
I have simplified the PR to include only documentation changes on function and vignette |
|
@osenan did you see this : p? #280 (comment) |
There was a problem hiding this comment.
I've completed the review, and similar to last time, I ended up revising almost all of the sentences for clarity and proper English structure. The content itself is excellent and technically sound, but the English phrasing was consistently difficult to follow.
I noticed that the sentence structure often seems to follow the pattern of your native language rather than standard English word order. It reads like a literal translation, which, unfortunately, doesn't account for the different grammar and flow English uses.
To help speed up this process and ensure the documentation is accessible to a wider English-speaking audience, I highly recommend using a tool like ChatGPT or another large language model (LLM) to double-check or even draft the text in English next time. These tools are excellent at refining translations to sound natural and professional in the target language.
I'm happy to keep reviewing the technical content, but using an LLM first will save both of us significant time on the language polish!
Let me know if you have any questions about my suggested changes.
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
Co-authored-by: Lluís Revilla <185338939+llrs-roche@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
|
I have read the CLA Document and I hereby sign the CLA |
|
@m7pr Many thanks for the review and comments. I will review better the documentation next time! |
# Pull Request Document expectation of what should return `qenv` with no code. Discovered while working on insightsengineering/teal#1630 where some code assumed it would return `NULL`. Previously it was only tested that `testthat::expect_identical(get_code(qenv(), names = "a"), "")`. It makes sense that it also return `""` when there is no code. --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Add copilot-setup-steps workflow using the reusable template from r.pkg.template. --------- Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Pull Request
It addresses this issue #278
While the reproducibility of
withinfunction is yet not supported with side effects functions, this PR will help users to make sure their code is reproducible or receive warning instead. In further issues we can debate whether we refactor within to support linking of side effects.To test, verify that the vignette is correctly updated, see if tests are passed.
If you create a qenv with this branch with this code, it should trigger a warning: